Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(diagnostics): workaround Rollup duplicating error messages #373

Merged
merged 2 commits into from
Jul 6, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jul 2, 2022

Summary

Adds a workaround for a Rollup bug that causes duplicate error messages

Details

  • per Duplicate error message (esp. when pretty: true) #103 (comment) and my investigation in fix(diagnostics): pretty defaults to true in TS 2.9+ #372, it seems like Rollup has a bug where it duplicates some error message

    • this occurs when the error has a stack (or frame) which contains the error message itself
      • Rollup prints both the error message and the stack in that case, causing duplication
  • this fix adds a workaround for this upstream Rollup bug

    • it detects if there is a stack and if the message is duplicated in the stack
      • if so, it removes the duplication in the stack
    • this workaround should be forward-compatible if this behavior is fixed upstream
      • this code should just end up re-throwing in that case (effectively a no-op)

Review Notes

This is gonna merge conflict pretty hard with #345 's current code (as these both add the buildEnd hook), but I made them both compatible with each other. Will just have to update one or the other when one is merged

Future Work

I'll make a PR to Rollup proper and link it here as well

- per my investigation in the linked issues, it seems like Rollup has a bug where it duplicates some error message
  - this occurs when the error has a stack (or frame) which contains the error message itself
    - Rollup prints _both_ the error message _and_ the stack in that case, causing duplication

- this fix adds a workaround for this upstream Rollup bug
  - it detects if there is a stack and if the message is duplicated in the stack
    - if so, it removes the duplication in the stack
  - this workaround should be forward-compatible if this behavior is fixed upstream
    - this code should just end up re-throwing in that case (effectively a no-op)
@agilgur5 agilgur5 added kind: bug Something isn't working properly solution: workaround available There is a workaround available for this issue scope: upstream Issue in upstream dependency labels Jul 2, 2022
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 2, 2022

sh*t, I was reading #103 (comment) and this seems to break watch mode for some reason.... back to the drawing board again....

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 2, 2022

Oh... well apparently Rollup needs some other properties that it attaches to the error:

{
    id: '/project-dir/src/difference.ts',
    hook: 'transform',
    code: 'PLUGIN_ERROR',
    plugin: 'rpt2',
    watchFiles: [
      '/project-dir/src/index.ts',
      '/project-dir/tsconfig.json',
      '/project-dir/src/types.ts',
      '/project-dir/src/difference.ts'
    ]
  }
}

So need to spread those in. And spreading them in makes watch mode work again, thankfully

- apparently Rollup attaches several properties to the error object, including `watchFiles`
  - so removing them / not spreading causes watch to just stop working

here are some of the additional properties I logged out, for example:
```js
{
    id: '/project-dir/src/difference.ts',
    hook: 'transform',
    code: 'PLUGIN_ERROR',
    plugin: 'rpt2',
    watchFiles: [
      '/project-dir/src/index.ts',
      '/project-dir/tsconfig.json',
      '/project-dir/src/types.ts',
      '/project-dir/src/difference.ts'
    ]
  }
}
```
@agilgur5
Copy link
Collaborator Author

This has been fixed upstream in Rollup in rollup/rollup#4749 and released in Rollup v3.7.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working properly scope: upstream Issue in upstream dependency solution: Rollup behavior This is Rollup's behavior and not specific to this plugin solution: workaround available There is a workaround available for this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate error message (esp. when pretty: true)
2 participants